WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy #2791

robskillington · 2020-10-22T09:34:36Z

What this PR does / why we need it:

This reduces a significant amount of all index query allocations by implementing the conjunction search as a lazy iterator over existing postings lists. It also caches regexp compilations with an LRU cache and allows setting different sizes for the regexp cache.

This change completely re-implements roaring bitmaps for query execution, this means pilosa roaring bitmaps are only used and index segment build time and all queries execute new read only roaring bitmaps which are completely mmap backed without any bitmap container/other allocations involved.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

arnikola

LGTM w nits

src/m3ninx/search/searcher/regexp.go

src/m3ninx/search/searcher/term.go

src/m3ninx/postings/types.go

src/m3ninx/search/searcher/all.go

src/m3ninx/search/searcher/lazy_postings_list.go

arnikola · 2020-10-22T14:31:21Z

src/m3ninx/search/searcher/disjunction.go

+	var (
+		union = make([]postings.List, 0, len(s.searchers))
+	)


nit: don't need var block here

src/m3ninx/search/searcher/conjunction.go

src/m3ninx/search/searcher/disjunction.go

src/cmd/services/m3dbnode/config/cache.go

…ings lists

arnikola

I had a cursory glance over but didn't go super deep on it, figuring a lot of it was very similar to Pilosa's implementation; if this is a bad assumption can go back with more discerning review

arnikola · 2020-10-26T15:20:15Z

src/cmd/services/m3coordinator/ingest/metrics.go

+	"github.com/uber-go/tally"
+)
+
+// LatencyBuckets are a set of latency buckets useful for measuring things.


nit measuring latencies

src/cmd/services/m3dbnode/config/cache.go

src/cmd/services/m3coordinator/ingest/metrics.go

src/cmd/services/m3coordinator/ingest/metrics_test.go

arnikola · 2020-10-27T00:20:16Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+func (i *multiBitmap) IsEmpty() bool {
+	iter := i.Iterator()
+	hasAny := iter.Next()
+	_ = iter.Err()


Hm, should this return false if iter.Err() is non-nil? Otherwise no need to call Err

arnikola · 2020-10-27T00:38:55Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+type containerIterator interface {
+	NextContainer() bool
+	ContainerKey() uint64
+	ContainerUnion(ctx containerOpContext, target *bitmapContainer)
+	ContainerIntersect(ctx containerOpContext, target *bitmapContainer)
+	ContainerNegate(ctx containerOpContext, target *bitmapContainer)
+}


nit: do these need to be exported? If so, may need comments (and may be worth exporting containerIterator?)

arnikola · 2020-10-27T01:19:47Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+			i.bitmap.Reset(true)
+
+			intersects := i.filter(i.multiContainerIter.containerIters, multiContainerOpIntersect)
+			negates := i.filter(i.multiContainerIter.containerIters, multiContainerOpNegate)


Doesn't this overwrite the i.filtered in intersects?

ah yes, TY!

arnikola · 2020-10-27T01:29:16Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+func (i *multiBitmapContainersIterator) NextContainer() bool {
+	if len(i.iters) != 0 {
+		// Exhausted.
+		return true


Should this be

if len(i.iters) == 0 { return false }

arnikola · 2020-10-27T01:45:15Z

src/x/cache/lru_cache.go

+
+	// Tell any other callers that we're done loading
+	if entry.loadingCh != nil {
+		close(entry.loadingCh)


nice pattern 👍

arnikola · 2020-10-27T14:56:13Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+		}
+		// Only valid if all intersecting actually matched,
+		// if zero intersecting then postings does not contain ID.
+		return len(i.intersect) > 0


Nit; can move this to 181, before checking intersectNegate lists

arnikola · 2020-10-27T15:13:48Z

src/m3ninx/postings/roaring/bitmap_readonly_multi.go

+		case elem.multiBitmap != nil:
+			it = elem.multiBitmap.containerIterator()
+
+		case elem.bitmap != nil:


nit: needs to fail if both set?

arnikola · 2020-11-03T04:12:47Z

src/m3ninx/postings/roaring/roaring_test.go

-
-	require.False(t, first.Equal(second))
-}
+// func TestRoaringPostingsListEqualWithOtherNonRoaring(t *testing.T) {


nit; delete?

arnikola · 2020-11-03T04:13:18Z

src/m3ninx/search/proptest/concurrent_test.go

@@ -39,16 +39,17 @@ import (

 func TestConcurrentQueries(t *testing.T) {
 	parameters := gopter.DefaultTestParameters()
-	seed := time.Now().UnixNano()
-	parameters.MinSuccessfulTests = 100
+	// seed := time.Now().UnixNano()


nit; can probably resume using rng seed

arnikola · 2020-11-03T04:13:41Z

src/m3ninx/search/proptest/concurrent_test.go

+	// simpleReader, err := simpleSeg.Reader()
+	// require.NoError(t, err)
+	// simpleExec := executor.NewExecutor([]index.Reader{simpleReader})


nit; delete

arnikola · 2020-11-03T04:14:16Z

src/m3ninx/search/proptest/concurrent_test.go

 			require.NoError(t, err)
 			matchedDocs, err := collectDocs(dOrg)
-			require.NoError(t, err)
+			require.NoError(t, err, fmt.Sprintf("query: %v\n", q.String()))


nit; require.NoError(t, err, q.String()) may be easier?

arnikola · 2020-11-03T04:19:09Z

src/m3ninx/postings/compare.go

+	if okA && okB && countA != countB {
+		return false
+	}
+


If using countfast here, we should also make sure that

if !okA && !okB { return EqualIterator(a.Iterator(), b.Iterator()) } return false

arnikola · 2020-11-03T04:24:00Z

src/m3ninx/postings/roaring/bitmap_multi_readonly.go

+	intersects []postings.List,
+	negates []postings.List,
+) (postings.List, error) {
+	intersect := make([]readOnlyIterable, 0, len(intersects))


nit: prealloc a slice of iterables := make([]readOnlyIterable, 0, len(intersects) + len(negates) and have both intersect and negate be taken from that, e.g. intersect, negate := iterables[:len(intersects)], iterables[len(intersects):]

arnikola · 2020-11-03T04:32:12Z

src/m3ninx/postings/roaring/bitmap_multi_readonly.go

+	Contains(id postings.ID) bool
+	ContainerIterator() containerIterator
+}
+
+type containerIterator interface {
+	NextContainer() bool
+	ContainerKey() uint64
+	ContainerUnion(ctx containerOpContext, target *bitmapContainer)
+	ContainerIntersect(ctx containerOpContext, target *bitmapContainer)
+	ContainerNegate(ctx containerOpContext, target *bitmapContainer)
+	Err() error
+	Close()


nit; comments here

arnikola · 2020-11-03T04:34:54Z

src/m3ninx/postings/roaring/bitmap_multi_readonly.go

+func (i *multiBitmap) IsEmpty() bool {
+	iter := i.Iterator()
+	hasAny := iter.Next()
+	_ = iter.Err()


do these need to check err?

…indexed snapshot during a GC run

…uld phase out

…s builder

…-index-query-allocs-rebase

…ding to rebuild all cached searches again

…terministic query key, also returned optimized form

robskillington and others added 2 commits October 22, 2020 05:31

[dbnode] Reduce index query allocations

f97ec5f

Merge branch 'master' into r/reduce-index-query-allocs

00462c1

arnikola approved these changes Oct 22, 2020

View reviewed changes

robskillington added 4 commits October 23, 2020 17:26

Use new LRU cache and instrument

17b7398

Revert go.mod

a6f05a7

Use read only bitmap by default in FST segment

c8683af

Implement read only bitmap for close to zero alloc operations on post…

a0f1801

…ings lists

arnikola self-assigned this Oct 26, 2020

arnikola reviewed Oct 27, 2020

View reviewed changes

robskillington and others added 8 commits October 27, 2020 17:36

Add tests

5208869

Fix tests

a76c6c4

Fix few issues

9592410

Fix multi segments builder

a0ce92f

Fix fieldsAndTermsIter

6786415

Fix checker

dd58315

Merge branch 'master' into r/reduce-index-query-allocs

cfac389

Fix multi-intersect searches that start with empty posting lists

fd8e4e5

robskillington changed the title ~~[dbnode] Reduce conjunction search and regexp compilation index query allocations~~ [dbnode] Refactor posting list operations to become zero copy and lazily evaluated Nov 1, 2020

robskillington changed the title ~~[dbnode] Refactor posting list operations to become zero copy and lazily evaluated~~ [dbnode] Refactor posting list operations to be lazily evaluated and zero copy Nov 1, 2020

Refactor read only bitmap range to own datastructure

f2688e0

arnikola reviewed Nov 3, 2020

View reviewed changes

robskillington added 5 commits January 26, 2021 20:11

Alloc GC segments each background GC segment run, also do not mutate …

5eb19b6

…indexed snapshot during a GC run

Reduce insert lock contention by spreading over many queues per CPU

ccb3035

4x the queues per CPU core

de3ce9a

Use series lookup as source of truth for if indexed or not and if sho…

834efa6

…uld phase out

Only run background GC when sealed blocks updated

3699014

robskillington force-pushed the r/reduce-index-query-allocs branch from c0e6836 to 3699014 Compare February 8, 2021 06:06

robskillington added 24 commits February 8, 2021 01:11

Remove purge expired series warning

4934faf

Use o(1) lookup for negative offsets for postings IDs in multisegment…

55632a9

…s builder

Load term FSTs up front to avoid slow queries during compactions

3b8c1d0

Fix CI tests

1c86639

Always consider the in-memory block

33590cc

Filter instead of shrink slice

6a0aa10

Only decrement readerwrite count if marking as success

9421bae

Always re-resolve entry when checking if should GC series

daed491

Merge commit 'e0bc12aeda246f2c39852d44ddb59a11cb3d9d31' into r/reduce…

3bc3b81

…-index-query-allocs-rebase

Avoid index block ticking while holding index write lock

3de1690

Rerun cached searches when replacing background segments to avoid nee…

da1aeb4

…ding to rebuild all cached searches again

Add concurrent execution of cached searches

0738d68

Initialize worker pool

1f8a510

Reuse cached postings when populating cached searches

9cf50ee

Fix search query proto

e151cbc

Fix missing postings for registry value

b886052

Add a postings cache TTL of 15min, also sort conjunction query for de…

a867722

…terministic query key, also returned optimized form

Fix build

b8f4d82

Restore simple LRU cache for accuracy when compacting segments

34a8594

Shard postings list cache

61e6185

Fix reconstructed searches not being executed

5e03570

Do not pool multi-bitmap iterators

734d67f

Add test for populate worker

43c0026

If tag name is a graphite tag then do not allocate each time

6379d0b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy #2791

WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy #2791

robskillington commented Oct 22, 2020 •

edited

Loading

arnikola left a comment

arnikola Oct 22, 2020

arnikola left a comment

arnikola Oct 26, 2020

arnikola Oct 27, 2020 •

edited

Loading

arnikola Oct 27, 2020

arnikola Oct 27, 2020

robskillington Oct 27, 2020

arnikola Oct 27, 2020

arnikola Oct 27, 2020

arnikola Oct 27, 2020

arnikola Oct 27, 2020

arnikola Nov 3, 2020

arnikola Nov 3, 2020

arnikola Nov 3, 2020

arnikola Nov 3, 2020

arnikola Nov 3, 2020

arnikola Nov 3, 2020 •

edited

Loading

arnikola Nov 3, 2020

arnikola Nov 3, 2020

WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy #2791

Are you sure you want to change the base?

WIP [dbnode] Refactor posting list operations to be lazily evaluated and zero copy #2791

Conversation

robskillington commented Oct 22, 2020 • edited Loading

arnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington commented Oct 22, 2020 •

edited

Loading

arnikola Oct 27, 2020 •

edited

Loading

arnikola Nov 3, 2020 •

edited

Loading